Do not panic on attempting to force-close a subchannel #151
Conversation
There was a problem hiding this comment.
I think your fix is fine. IIRC originally this path could never be hit, but probably due to changes in the way the periodic_check is implemented it does now. What the proper fix is is a bit of an open question, last time we discussed about that there was no really conclusion. For the "regular" DLC channel force closing in case of a non responsive peer during channel establishment/update make sense to me, but it might not be the best for the DLC/LN ones.
(Note the target branch is not the "main" ln/dlc one, not sure if that's on purpose)
…-closure attempt We've run into `unimplemented!` block in rust-dlc in our regtest setup. In order to unblock the regtest environment, this quickfix logs an error and continues instead of panicking. See: p2pderivatives/rust-dlc#151
klochowicz
changed the base branch from
feature/ln-dlc-channels-10101
to
feature/ln-dlc-channels
We have run into this in production, it seems logging an error instead of panicking is safest.
klochowicz
force-pushed
the
fix/do-not-panic-on-subchannel-force-closure
branch
from
ff3eae8 to
6775c53
Compare
thank you for your feedback, I've retargeted this PR into the "main" ln/dlc one ( I wonder whether I should log an error or just ignore the subchannel instead of an error if it will become more frequent 🤔 - for now I'll deploy it and see how it behaves. |
…-closure attempt We've run into `unimplemented!` block in rust-dlc in our regtest setup. In order to unblock the regtest environment, this quickfix logs an error and continues instead of panicking. See: p2pderivatives/rust-dlc#151
Yeah I agree maybe warn or info level could be enough (or not having a log at all would also be fine, depends if you think the info is useful or not). Also you can cherry pick this commit to fix clipppy: ecbc386 (might not apply perfectly though since it's for the master branch which doesn't have sub channels). |
| @@ -129,7 +129,10 @@ macro_rules! check_for_timed_out_channels { | |||
| let is_timed_out = timeout < $manager.time.unix_time_now(); | |||
| if is_timed_out { | |||
| let sub_channel = if channel.is_sub_channel() { | |||
| unimplemented!(); | |||
| error!("Force-closing subchannels is not supported; skipping closure."); | |||
There was a problem hiding this comment.
To be more precise, what's not supported is force-closing timed out subchannels, but AFAIK we could force-close here if we wanted to.
In 10101 we probably want more control over triggering a force-close, so I think we just don't want to call check_for_timed_out_channels at all. I wonder if this feature of the library is actually useful for any consumer, since force-closing is often an expensive decision cc @Tibo-lg. AFAIK this timeout is not based on the refund timelock, so we're being overeager.
I agree with removing the unimplemented, but I would maybe just log on Debug or nothing at all.
|
Superseded by #159. |
We have run into this in our public regtest setup, it seems logging an error instead of panicking is safest, as at least it won't bring down the whole app (and it will instead spam the logs).
this is just a quickfix and will not solve the underlying issue.
@Tibo-lg there's come code commented out there - I assume you had some reasons to not try to keep it as is - therefore before jumping into trying to fix this issue properly, I'd like to hear your opinion if you had something in mind :)